TabletManager: Check for context cancellation in loop within ChangeTabletType() #9842
Conversation
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
| @@ -186,6 +186,9 @@ func (ts *tmState) ChangeTabletType(ctx context.Context, tabletType topodatapb.T | |||
| // We must read the data again and verify whether the previous write succeeded or not. | |||
| // The only way to guarantee safety is to keep retrying read until we succeed | |||
| for { | |||
| if ctx.Err() != nil { | |||
| return ctx.Err() | |||
There was a problem hiding this comment.
We had added the infinite loop intentionally, because we enter that code only if a topo write has failed. Once a topo write fails, the only way forward is to keep trying to read the topo server, because the information may or may not have been written.
However, I do understand the use case where they want to retry doing the same operation again, so we need to exit sometime and not wait indefinitely.
I just think we should wrap the returned error and explicitly say that the topo information might be different from the tablet information and that the user must call ChangeTabletType again to fix it.
Other than that, this looks good to me
There was a problem hiding this comment.
I just think we should wrap the returned error and explicitly say that the topo information might be different from the tablet information and that the user must call ChangeTabletType again to fix it.
Suggestions for the error message? return fmt.Errorf("context canceled updating tablet_type for %s in the topo, please retry", ts.tm.tabletAlias) ok?
There was a problem hiding this comment.
Yes I think that will work!
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
… within ChangeTabletType() (#10035) * Check for context cancellation in loop within ChangeTabletType() Signed-off-by: Rohit Nayak <rohit@planetscale.com> * Change error message Signed-off-by: Rohit Nayak <rohit@planetscale.com>
Description
There is currently the possibility of an infinite loop inside
ChangeTabletType()because we don't check for context cancellation inside an active polling loop. This PR adds the check.Related Issue(s)
#9838
Checklist